fix: prevent duplicate dev fee payment on timeout#584
Conversation
On timeout, the code was blindly resetting dev_fee_paid=false and
clearing dev_fee_payment_hash. However, a timeout does NOT mean the
Lightning payment failed — it could still be in-flight and may succeed
after the timeout window. This caused a race condition where a second
payment would be initiated on the next scheduler cycle.
Changes:
- Remove state reset on timeout: leave the order in PENDING state
(dev_fee_paid=true) so it won't be picked up for a duplicate payment
- Add timestamp to PENDING marker (PENDING-{uuid}-{unix_ts}) for
accurate staleness detection
- Replace taken_at-based stale detection with timestamp-based parsing
from the PENDING marker itself
- Legacy markers without timestamps are treated as stale for backward
compatibility
- 7 unit tests for the timestamp parser
The stale PENDING cleanup (5-min TTL) serves as the safety net: if the
payment never resolves, the order is reset after the TTL expires.
Closes #568
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scheduler.rs (1)
517-555:⚠️ Potential issue | 🟡 MinorBroken doc comments:
parse_pending_timestampinherits stale docs fromjob_process_dev_fee_payment.The new function was inserted in the middle of the existing doc comment block for
job_process_dev_fee_payment. As a result:
parse_pending_timestamp(Line 527) gets a misleading///doc starting with "Processes unpaid development fees…" (Lines 517-519, leftover from the old function).job_process_dev_fee_payment(Line 555) is left with an orphaned doc fragment starting mid-list at "- status = 'settled-hold-invoice'…" (Line 546).
cargo docwill render both incorrectly.Proposed fix: move the function and clean up both doc blocks
Move
parse_pending_timestampabove thejob_process_dev_fee_paymentdoc block, or fix the docs in place:-/// Processes unpaid development fees for completed orders -/// -/// Runs every 60 seconds and attempts to pay dev fees for orders that have: /// Parse the unix timestamp from a PENDING marker. /// /// Marker format: `PENDING-{uuid}-{unix_timestamp}` /// Legacy format: `PENDING-{uuid}` (no timestamp) → returns `None` /// /// Returns `Some(timestamp)` if a valid unix timestamp is found at the end, /// `None` otherwise. fn parse_pending_timestamp(marker: &str) -> Option<u64> { // ... } +/// Processes unpaid development fees for completed orders. +/// +/// Runs every 60 seconds and attempts to pay dev fees for orders that have: /// - status = 'settled-hold-invoice' OR 'success' /// - dev_fee > 0 /// - dev_fee_paid = false /// /// Design decisions: /// ... async fn job_process_dev_fee_payment() {
🧹 Nitpick comments (2)
src/scheduler.rs (2)
564-571: Hardcoded TTL and inconsistent time source.Two minor points:
cleanup_ttl_secsis hardcoded to300. Other timing values in this file (e.g.,exp_seconds,interval) are pulled fromSettings. Consider making this configurable alongside those.The new code uses
std::time::SystemTime::now()(Lines 568, 692) while the rest of the file consistently useschrono::Utc::now(). Not a correctness issue, but an inconsistency worth aligning.
836-838: Test module should be namedtestsper project convention.The coding guidelines specify: "Co-locate tests in their modules under
mod tests." The current namepending_marker_testsdeviates from this convention.♻️ Rename the module
#[cfg(test)] -mod pending_marker_tests { +mod tests { use super::parse_pending_timestamp;As per coding guidelines: "Co-locate tests in their modules under
mod testswith descriptive names likehandles_expired_hold_invoice."
Replace std::time::SystemTime::now() with Utc::now().timestamp() to align with the rest of the file. Also fix doc comment placement and rename test module.
2 participants
Problem
When a dev fee Lightning payment times out (50s), the code blindly resets
dev_fee_paid = falseand clearsdev_fee_payment_hash. However, a timeout does not mean the payment failed — it could still be in-flight on the Lightning Network and may succeed after the timeout window.This creates a race condition:
Solution
On timeout, leave the order in PENDING state instead of resetting.
The stale PENDING cleanup (5-min TTL) serves as the safety net:
Changes
dev_fee_paid=truewith PENDING marker so the order is not picked up for duplicate paymentPENDING-{uuid}-{unix_ts}for accurate staleness detectiontaken_at-based stale detection — parse timestamp from the PENDING marker itself (fixes the wrong-timestamp issue from Stale PENDING Detection Uses Wrong Timestamp Field #570)Verification
cargo fmt✅cargo clippy --all-targets --all-features✅ (0 warnings)cargo test✅ (119 passed)Closes #568
Related: #570 (stale detection timestamp fix)
Summary by CodeRabbit
Bug Fixes
Tests